Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve Switzerland #195

Closed

Conversation

herpaderpaldent
Copy link

Improve Switzerland

This pull requests aims to resolve #193 and adds various imrovements whilst refactoring the code to make it more maintainable.

Whilst the original implementation relied on wikipedia. This pull request implements the holidays communicated by the federal bureau of justice FOJ.

  • maintainability: refactor regions/cantons
  • refactor: instead of maintaining a large array, the regions/cantons are split in individual functions making them easier to maintain and update. This allows for further inheritance for later implementation of districts and cities.
  • support more advanced regional holiday calculations
  • add labour day
  • add rumantsch as language
  • maintainability: use English as default language (not German)
  • add more tests

herpaderpaldent and others added 2 commits January 29, 2024 20:10
- maintainability: refactor regions/cantons
- support more advanced regional holiday calculations
- add labor day
- add rumantsch as language
- maintainability: use english as default language (not german)
- add more tests
@Nielsvanpach
Copy link
Member

maintainability: use English as default language (not German)

I do prefer it to return the most general language by default.

$holidays = Holidays::for('ch')->get(); // German by default instead of English

@@ -7,6 +7,42 @@
use Spatie\Holidays\Exceptions\InvalidRegion;
use Spatie\Holidays\Holidays;

dataset('cantons', [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think all cantons need a specific snapshot test. You can only verify the edge cases, which is more explicit.

->toBeArray()
->not()->toBeEmpty();

expect(formatDates($holidays))->toMatchSnapshot();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for a big snapshot tests here. You can just verify if a certain holiday name is present.

})->with('languages');

it('does use native language for epiphany', function (string $canton, string $name, string $locale) {
CarbonImmutable::setTestNowAndTimezone('2024-01-01');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
CarbonImmutable::setTestNowAndTimezone('2024-01-01');
CarbonImmutable::setTestNow('2024-01-01');

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be found multiple times in the tests

@Nielsvanpach
Copy link
Member

refactor: instead of maintaining a large array, the regions/cantons are split in individual functions making them easier to maintain and update.

I don't really agree this solution is more readable and easier to maintain

@Nielsvanpach
Copy link
Member

Thanks for your efforts and feel free to reopen if you're still planning to update this PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Swiss Regional Holidays are incorrect
2 participants